Fix duplicate daily wiki metrics by making the write idempotent - #1268

I don't think this is required because the job already implements the ShouldBeUnique interface. Have a look at the Laravel docs and let me know what you think.

I checked out the docs. ShouldBeUnique and withoutOverlapping are 2 different mechanisms. Basically “don’t run simultaneously” vs “don’t queue duplicates.”

• ShouldBeUnique is a queue-level "deduplication", it prevents the same queued job from being dispatched again while the same unique job is still running.
• withoutOverlapping() is a scheduler-level guarantee, it prevents the same scheduled task from running concurrently even if the same task is triggered again or starts late.

I would say that they are not strictly redundant. I would like to convince you to embrace the conservative/default Laravel pattern and keep the withoutOverlapping() here unless you think the job "should be unique" only at queue-level.

https://laravel.com/framework/docs/11.x/scheduling#preventing-task-overlaps
https://laravel.com/framework/docs/11.x/queues#unique-jobs

I also went through the docs and it is true that this 2 are different but as I understand ShouldBeUnique prevents the job running if there is already one in process. while withoutOverlapping() prevents the job from being scheduled if there is already one in process. It seems to me for this use case, Ollie is right about both not being required.

I was almost convinced that we should remove withoutOverlapping(), but:

If we remove withoutOverlapping(), we would be relying on the job’s internal idempotence check as the only protection against overlapping. This is not the same idea as preventing the same scheduled task from running in parallel at the task scheduler.

withoutOverlapping() enforces single execution and ShouldBeUnique ensures correctness. I think they complement each other rather than replace one another

Thanks both for looking into this!

@dati18 I was almost convinced that we should use both ShouldBeUnique and withoutOverlapping(), however, the Laravel docs alone weren't giving me enough information to come to a decision. Specifically, I wanted to know:

• At what point in the process does each of them get evaluated?
• What happens in each case when a job is already queued or running (i.e. does the job stop or does it wait for the first one to finish)?

To help, I read some blog posts:

• https://devinthewild.com/article/should-be-unique-vs-without-overlapping-laravel-10
• https://mozex.dev/blog/25-shouldbeunique-vs-withoutoverlapping-neither-sets-a-lock-expiry-for-you

My conclusions are:

• A ShouldBeUnique Job is aborted, before it can be queued, if there is already a queued or running Job with the specified uniqueId.
• ... I didn't get time to finish this yet - I'll come back and edit this comment, but wanted to share where I had got to so far before I jump in another meeting.

This is all pretty confusing stuff to get our heads around. We should be careful not to fall down a rabbit hole! 🐰 🕳️

I spent some time reading these 2 articles you linked. And I think I can agree on one thing: ShouldBeUnique and withoutOverlapping() are related, but they are not interchangeable.

The stronger point from the second article is that neither one gives you a safe default expiry:

• on Redis, the lock can have no expiry at all (unless you configure one)
• on the database lock store, there may be a default timeout, but it is still not something I can assume is correct for this workload
• if a worker dies mid-job, the lock can remain stranded
  So in practice, both locks are only as good as lock lifetime and failure handling allow them to be.

Some reflection on what I did:

• withoutOverlapping() is a useful operational guard
• ShouldBeUnique is a useful queue guard
• And the real guard is idempotent writes at the data layer (WikiMetrics.php)

To answer @outdooracorn 's question:

At what point in the process does each of them get evaluated?

• ShouldBeUnique is evaluated when the job is being dispatched/queued. Laravel checks it before the job is added to the queue. If the unique lock already exists, Laravel does not enqueue the duplicate, and the second dispatch is discarded -> So the decision happens at dispatch time, not when the worker starts processing.
• withoutOverlapping() is evaluated when the scheduler decides it's time to run a scheduled job. The scheduler (Kernel) tries to acquire a mutex for that task. If the lock already exists, the scheduled run is skipped, and it does not wait for the 1st one to finish -> So the decision happens at scheduler execution time, not at queue dispatch time.

What happens when a job is already queued or running?

• ShouldBeUnique: job is not queued, 2nd dispatch is dropped, 1st job continues normally
• withoutOverlapping(): the new run is skipped and not queued behind the 1st run

For this use case, I think keeping both is the right choice. The reason is not that they are equivalent, but that they protect different layers. The important part is data correctness, not just “one job instance” or picking the best duplicate-defense mechanism for it.

While it's interesting and fun to research about it, it's a bit tiring reading this much stuff in a day just to come up with a good argument to defend my decision. But I still believe in a robust design and extra protection.

There are some interesting things in there @dati18. I haven't had time to read or process them fully. I believe I agree and came to the same conclusion as you on some of your points, and am not so sure about others! 😅

Another blog I started reading: https://romanzipp.com/blog/unique-job-processing-in-laravel

However, trying to move this PR along. Given that:

• we haven't come to a concrete understanding of what this does
• the job has been running for a while without the withOverlapping() method being used when scheduled
• checking if a record exists in the database before collecting any metrics should remove the logspam which is the goal of this task
• this isn't our focus topic

I think we should:

• remove this withOverlapping() method change from the PR in order to complete this task
• create another task with all the information (along with sources and evidence) that we have collected so far, that we can either continue investigating now or save for the future

I'll just remove it as you wish. We can either add it or we don't; there's no need for a whole new ticket for something trivial as adding withoutOverlapping()

Would be nice to add another test to be sure it logs the warning when there is already a record for the given day.

I used the LogFake pattern, same pattern in our repo ;) please check